Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI Tests: Permissions #2625

Merged
merged 158 commits into from
Apr 15, 2024
Merged

UI Tests: Permissions #2625

merged 158 commits into from
Apr 15, 2024

Conversation

Halle
Copy link
Contributor

@Halle Halle commented Apr 15, 2024

Task/Issue URL: https://app.asana.com/0/1199230911884351/1205717021705373/f
Tech Design URL:
CC:

Description: Adds a series of UI tests for permissions

Steps to test this PR:

  1. Open the scheme UI Tests
  2. Navigate to the test pane
  3. Run PermissionsTests

UI Tests general guidelines for everyone:

  • It isn't possible to multitask while running UI tests on your local machine, because your mousing/interface usage or the unknown focus events of other in-use applications can change, slow down, or intercept screen interactions that the tests depend on. Multitasking includes being on calls, and includes work in a different space. Tedious as it is, the UI tests are the only thing you can do with your computer when you test whether UI tests pass, because that is the only circumstance that XCUIAutomation effectively supports locally (this makes sense if you consider how closed the simulator environment where this API gets the most development is, by contrast with an engineer's development computer). This approach keeps the debug loop fast and the tests free of workaround buildup.
  • Due to app-specific text field focus issues related to waiting for focus across two monitors simultaneously, we've decided to review the tests on a single monitor.
  • English is the currently supported language for UI tests
  • The tests have been tested on multiple systems before the PR is opened. If you experience a failure when testing locally, in order to confirm that it isn't a false negative failure, please take the following steps before reporting it:
  1. If you weren't watching when it happened (probably not, it's pretty tedious) please re-run the failed case and observe the failure. This does a few things: it means the person most acquainted with the system where the test failed can describe what they observed and knows what the test tries to do, which will promote a faster and more minimal solution, and it is a way of ruling out accidental multitasking, which can happen if a call comes in or a long GUI process elsewhere concludes. If a failure recurs when you observe the test case, please report it.
  2. When reporting it, please send the xcresult bundle, even if it seems like the same failure as a previous failure (it may be subtly different). Since the xcresult bundle will include a screen recording or screenshots, for your privacy, please consider the visibility of things in your system you may not want to send to a contractor when you are running the tests.

Thank you very much for your help!

Halle added 30 commits March 14, 2024 16:07
Copy link
Contributor

github-actions bot commented Apr 15, 2024

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against 9c5ee24

@@ -25,6 +25,7 @@ class AddressBarKeyboardShortcutsTests: XCTestCase {

private var addressBarTextField: XCUIElement!
override class func setUp() {
UITests.firstRun()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we always start tests with a fresh install, every time we start a case, we also have to quickly make sure there are no first run problems like permissions which always appear, or the onboarding page.

app.typeKey("n", modifierFlags: .command)
app.typeKey("w", modifierFlags: [.command, .option])
app.terminate()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove fresh-install first-run startup nuisances for all test cases.

)
self.typeURL(url, pressingEnter: pressingEnter)
}

func clickAfterExistenceTestSucceeds() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New convenience function.

@@ -246,7 +246,7 @@ class FindInPageTests: XCTestCase {
XCTAssertEqual(statusFieldTextContent, "1 of 4", "Unexpected status field text content after a \"Find in Page\" operation.")

let webViewWithSelectedWordsScreenshot = loremIpsumWebView.screenshot()
let highlightedPixelsInScreenshot = webViewWithSelectedWordsScreenshot.image.matchingPixels(of: .findHighlightColor)
let highlightedPixelsInScreenshot = try XCTUnwrap(webViewWithSelectedWordsScreenshot.image.matchingPixels(of: .findHighlightColor))
XCTAssertGreaterThan(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Standardize unwrap asserts over tests.

@Halle Halle requested a review from ayoy April 15, 2024 13:15
Copy link
Collaborator

@ayoy ayoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks a lot @Halle! This worked great on my local machine with the exception of test_locationPermissions_whenAlwaysDenyIsSelected_alwaysDenies that left a system location permission prompt on the screen, but still passing all checks 👍

Let's handle it separately, and merge this for now. 🚢

@Halle
Copy link
Contributor Author

Halle commented Apr 15, 2024

LGTM! Thanks a lot @Halle! This worked great on my local machine with the exception of test_locationPermissions_whenAlwaysDenyIsSelected_alwaysDenies that left a system location permission prompt on the screen, but still passing all checks 👍

Let's handle it separately, and merge this for now. 🚢

Yes, I think this will be helpful to have in the tests for now, but the location TCC behavior can definitely be improved, so I will add a subtask explaining that in the project and merge. Thanks very much for checking it out!

@ayoy ayoy merged commit 9aaa05e into main Apr 15, 2024
15 of 17 checks passed
@ayoy ayoy deleted the halle/permissions-tests branch April 15, 2024 18:04
diegoreymendez pushed a commit that referenced this pull request Apr 18, 2024
Task/Issue URL: https://app.asana.com/0/1199230911884351/1205717021705373/f

Description: Adds a series of UI tests for permissions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants